Skip to content

Conversation

ViktorTigerstrom
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom commented Jun 10, 2025

Currently based on #1079

Implements step 1. of "Phase 3" in #917.

This is a draft PR to updates litd to use the new sqldd/v2 package. Note that this with just this PR, litd will not utilize the capabilities of sqldb v2 to run specific post migrations steps (such as migrating the kvdb to SQL). That functionality will be added in upcoming PRs.

Instead, this PR just focuses on adding support for the new sqldb v2 package, and the functionality of the SQL stores are expected to remain the same as prior to this commit.

Note that this is only a draft PR for now, as the sqldb/v2 is not released, and we therefore need to add a forked dependency in order to run this. Once we have an official release of the sqldb/v2 package, I'll update this PR to use that dependency.

@ellemouton
Copy link
Member

@ViktorTigerstrom - maybe change the base branch so we only see the relevant commits here?

In upcoming commits, we will introduce support for sqldb/v2 in Lightning
Terminal, in order to support code migrations. This is needed, so that
we can ensure that the kvdb to sql migration is run exactly after an
exact sql migration version, and not after all sql migrations have been
executed. The reason why that's important is that sql table definition
may change with future sql migrations, while the kvdb to sql migration
will remain the same and always expect the same table definition.
Therefore, if the kvdb to sql migration is run after all sql migrations,
it may fail due to a table definition mismatch.

This commit adds the sqldb/v2 dependency to `litd`, so that we can
implement it's usage and execution of the kvdb to sql migration
correctly in the upcoming commits.

NOTE: currently the sqldb/v2 dependency hasn't been shipped in any
lnd release, so this dependency currently a forked version of
sqldb/v2.
A core component of `sqldb/v2` useage, is that the package allows and
requires that the callsite defines a `sqldb.MigrationStream` that will
be run during the initialization of the `sqldb/v2 database instance.
The `sqldb.MigrationStream` defines the exact sql migrations to run,
as well as additional code migrations will be run after each individual
migration version.

This commit introduces the core definition of the migration stream for
`litd`, and will be further exteded in upcoming commits that will
introduce kvdb to sql code migration.

Note that as of this commit, as no part of the `litd` codebase uses the
`sqldb/v2` database instances, this migration stream is not yet
used.
The `sqldb/v2` package now provides a definition for the `BackendType`
type. As useage of `sqldb/v2` requires useage of that type, we update
`litd` to use the `BackendType` definition from `sqldb/v2`, instead of
it's own definition.
The upcoming implementation of `sqldb/v2` will extensively create
a `Queries` object on the fly. To make more intuitive how to create
the queries object for specific database types, we introduce a
`NewForType` helper method.
This also mimics how `tapd` creates the `Queries` object, and in order
not let `litd` have it's own definition of how `Queries` object are
created on the fly, the upcoming `sqldb/v2` usage will utilize this
helper method.
As we will change the `accounts`, `session` & `firewalldb` packages to
use the `sqldb/v2` package, we need to make those packages use the
`sqldb/v2` `PostgresStore` when setting up their test postgres
databases, instead of `litd`'s own `PostgresStore` version.

In order to enable that functionality, we add a new helper function that
creates a `PostgresStore` using the `sqldb/v2` package, in addition to
helper function that creates a `PostgresStore` using the `litd`
version.

Once we have shifted all of `litd`'s code to use the `sqldb/v2`
definition, we will remove the `litd` version.
Update the accounts package to use the `sqldb/v2` package instead of the
older version.
Update the session package to use the `sqldb/v2` package instead of the
older version.
Previous commits had forgotten to add the `ListAllKVStoresRecords` query
to the `firewalldb.SQLKVStoreQueries` interface. As that is required to
make the query useable when defining the `sqldb/v2`
`TransactionExecutor` for the `firewalldb` package, this commit
adds it to the interface.
rename `sqlStore` to `store` in the firewalldb sql migration test file,
to make the name shorted. This is done in preparation for future commits
which will lengthen the lines where `sqlStore` is used, which otherwise
would make the lines exceed the 80 character limit.
Update the firewalldb package to use the `sqldb/v2` package instead of
the older version.
As we've now switched over to using sqldb v2 for most of the db objects,
we can remove a lot of deprecated code that's no longer used in the litd
project. This commit removes that code.
As the legacy `NewTestPostgresDB` function is no longer used and has
been removed, it no longer makes sense to have a `V2` suffix on the
`NewTestPostgresV2DB` function. This commit renames it to
`NewTestPostgresDB`, to indicate that this function now replaces the
legacy function.
@ViktorTigerstrom ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Aug 28, 2025
@ViktorTigerstrom
Copy link
Contributor Author

@bitromortac & @ellemouton, I just requested reviews from both of you as I think we're ready to start reviewing this PR now.
Note though that I'll keep this in draft until we have lightningnetwork/lnd#10175 merged into a separate branch in lnd.

Therefore we can't merge this PR yet, as the dependency link will change when the above has merged. The actual code in this PR is final though, so it is worth reviewing IMO :)

Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, this PR is absolutely impeccable: really great descriptions, super modular commits, very easy to review - great work! 🔥

@@ -1,311 +1,5 @@
package db

import (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

Copy link
Contributor

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, this PR is absolutely impeccable: really great descriptions, super modular commits, very easy to review - great work! 🔥

100%, only a first pass for me (and I don't have a good view of the upstream work as well). Thanks for the great commit structure! 🚀

SQLQueries
}

func NewSQLQueriesExecutor(baseDB *sqldb.BaseDB,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we don't need to export those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR is does not require a release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants